fix: ensure bearer_token respects token_from
#1189
Conversation
TamerShlash
changed the title
bearer_token Respects token_frombearer_token Respects token_from
TamerShlash
changed the title
bearer_token Respects token_frombearer_token respects token_from
TamerShlash
force-pushed
the
fix/token_from-bearer_token
branch
from
ca5e7d2
to
cb8d3b3
Compare
|
Hello @aeneasr 👋 can you take a look at this? |
There was a problem hiding this comment.
Thanks for chiming in here! However, I'm not quite sure if this is the correct way to solve the problem.
The basic problem is that the bearer authenticator handles token_from in an unexpected way: the token is forwarded, but with the same method as defined in token_from.
Your fix adds the token to the forwarded request in the authorization header, however the original header is also forwarded. This fixes where the session store expects a Authorization: bearer <token>, however it does not solve other cases.
What do you think of adding a new config option to the authenticator, basically forward_token_as that is the inverse of token_from. With this approach we can ensure that we don't break anyone relying on the current behavior, but also don't forward the token twice.
I would not want to forward the token twice, as the token store might not correctly strip the token from logs etc. when it receives "just another extra header".
WDYT?
|
Hi @zepatrik! thank you for taking a look at this. I think the main misunderstanding here is that in my mind a session store was equal to Ory Kratos, which expects the session token exclusively in the If that's what you mean, that seems totally reasonable, and the fix you suggested with My only take on this is that Oathkeeper authenticators configuration has already been quite confusing to understand to begin with (maybe just for us), so I think we need to properly document how this new How do you want to go forward with this? I'm happy to update this PR with that suggestion if but can't tell how soon it will be since it's a bit busy for me this period and this is my first time working with Go ever 😄 Otherwise of course it's great if you want to take care of it. |
The Problem
The
bearer_tokenauthenticator always returns 401 when providing anytoken_fromoption. The reason is because the token is indeed detected, and therefore the authenticator is deemed responsible, but the token is not actually passed forward to the session store in theAuthorizationheader as the instructions here and here say should be done.The only exception when it does work is when using the default
Authorization: bearer <session-token>option (i.e, not specifyingtoken_fromat all), because theAuthorizationheader is forwarded anyways as you can see here:oathkeeper/pipeline/authn/authenticator_bearer_token.go
Line 124 in acb2584
This PR fixes that by making sure to set
Authorization: bearer <token>for the request going to the sessions store if a token is detected.Is It Breaking?
No, it should not be.
Related issue(s)
#1144
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.